Skip to content

🐛 Celery tasks aborted after submission #8371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Sep 16, 2025
Merged

🐛 Celery tasks aborted after submission #8371

merged 10 commits into from
Sep 16, 2025
+34 −10

Conversation

@giancarloromeo
Copy link
Contributor

@giancarloromeo giancarloromeo commented Sep 16, 2025 •
edited
Loading

What do these changes do?

This PR fixes a bug in Celery task submission by reordering operations and adding proper error handling to prevent orphaned task records in the task store.

  • Moves task store creation after Celery task submission
  • Adds exception handling to clean up task store records if submission fails
  • Ensures task store consistency by removing records when Celery submission fails

In some cases, the task record hadn't been written yet, and the task had been executed by the worker. This caused a premature abort due to the new abort_monitor mechanism.

Related issue/s

How to test

Dev-ops

@giancarloromeo giancarloromeo added this to the Cheops milestone Sep 16, 2025
@giancarloromeo giancarloromeo self-assigned this Sep 16, 2025
Copilot AI reviewed Sep 16, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in Celery task submission by reordering operations and adding proper error handling to prevent orphaned task records in the task store.

  • Moves task store creation after Celery task submission
  • Adds exception handling to clean up task store records if submission fails
  • Ensures task store consistency by removing records when Celery submission fails

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Sep 16, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.76%. Comparing base (5a5b417) to head (33abdf1).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8371      +/-   ##
==========================================
+ Coverage   87.86%   89.76%   +1.90%     
==========================================
  Files        1947     1331     -616     
  Lines       75663    56246   -19417     
  Branches     1322      203    -1119     
==========================================
- Hits        66484    50492   -15992     
+ Misses       8782     5687    -3095     
+ Partials      397       67     -330
Flag Coverage Δ
integrationtests 64.00% <ø> (+0.08%) ⬆️
unittests 87.96% <53.84%> (+1.42%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library 83.58% <53.84%> (-1.13%) ⬇️
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 84.99% <ø> (ø)
agent 93.53% <ø> (ø)
api_server 91.93% <ø> (ø)
autoscaling 95.77% <ø> (ø)
catalog 92.36% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 92.37% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 75.81% <ø> (ø)
director_v2 90.93% <ø> (+0.01%) ⬆️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.46% <ø> (ø)
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.61% <ø> (ø)
resource_usage_tracker 92.13% <ø> (+0.37%) ⬆️
storage 86.66% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 87.98% <ø> (+<0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5b417...33abdf1. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@giancarloromeo giancarloromeo marked this pull request as ready for review September 16, 2025 09:32
GitHK
GitHK approved these changes Sep 16, 2025
View reviewed changes
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

sanderegg
sanderegg reviewed Sep 16, 2025
View reviewed changes
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this fix the issue we see currently in master(s)?

@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2025 •
edited
Loading

🧪 CI Insights

Here's what we observed from your CI run for 33abdf1.

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI integration-tests Base branch is healthy, but retries were needed. Could be early signs of flakiness 👀 Healthy 1 View View
unit-tests Base branch is healthy, but retries were needed. Could be early signs of flakiness 👀 Healthy 1 View View

@giancarloromeo giancarloromeo enabled auto-merge (squash) September 16, 2025 10:44
@giancarloromeo
Copy link
Contributor Author

giancarloromeo commented Sep 16, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2025 •
edited
Loading

queue

🟠 Waiting for conditions to match

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = deploy to dockerhub
        • check-skipped = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-neutral = system-tests
        • check-skipped = system-tests
        • check-success = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = unit-tests
        • check-skipped = unit-tests
        • check-success = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
        • check-success = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-neutral = integration-tests
        • check-skipped = integration-tests
        • check-success = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
        • check-success = build-test-images (frontend) / build-test-images
      • any of: [🛡 GitHub branch protection]
        • check-neutral = SonarCloud Code Analysis
        • check-skipped = SonarCloud Code Analysis
        • check-success = SonarCloud Code Analysis
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@giancarloromeo giancarloromeo added the 🤖-automerge marks PR as ready to be merged for Mergify label Sep 16, 2025
sanderegg
sanderegg approved these changes Sep 16, 2025
View reviewed changes
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

pcrespov
pcrespov approved these changes Sep 16, 2025
View reviewed changes
Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, i do not want to hold this PR more ...
IMO the logger.exception is not really necessary and should be caller that logs the exception raised.

In short: in general do not log.exception if you are raising it, instead create a new custom exception and append extra context and reraise. Whoever full handles it (i.e. does not re-reaise it) will get a very rich context of what happened and cant take action.

@giancarloromeo
Copy link
Contributor Author

giancarloromeo commented Sep 16, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2025 •
edited
Loading

queue

🛑 Configuration not compatible with a branch protection setting

The branch protection setting Require branches to be up to date before merging is not compatible with max_parallel_checks>1, queue_conditions != merge_conditions and must be unset.

bisgaard-itis
bisgaard-itis approved these changes Sep 16, 2025
View reviewed changes
Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix

@giancarloromeo giancarloromeo changed the title 🐛 Fix Celery task submission 🐛 Celery tasks aborted after submission Sep 16, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 16, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@giancarloromeo giancarloromeo merged commit 4600d77 into ITISFoundation:master Sep 16, 2025
143 of 148 checks passed
This was referenced Sep 18, 2025
Closed
Closed
@mrnicegyu11 mrnicegyu11 mentioned this pull request Sep 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@GitHK GitHK GitHK approved these changes

@pcrespov pcrespov pcrespov approved these changes

@sanderegg sanderegg sanderegg approved these changes

@matusdrobuliak66 matusdrobuliak66 matusdrobuliak66 approved these changes

@bisgaard-itis bisgaard-itis bisgaard-itis approved these changes

Assignees

@giancarloromeo giancarloromeo

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:celery-library

Projects

None yet

Milestone

Cheops

Development

Successfully merging this pull request may close these issues.

6 participants

@giancarloromeo @GitHK @pcrespov @sanderegg @matusdrobuliak66 @bisgaard-itis